Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10078018:
Final(?) eradications of GDK / GTK from the GPU process. (Closed)
|
Created:
8 years, 8 months ago by Chris Evans Modified:
8 years, 8 months ago CC:
chromium-reviews, sadrul, erikwright (departed), jam, joi+watch-content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFinal(?) eradications of GDK / GTK from the GPU process.
TBR=darin
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133285
Patch Set 1 #
Total comments: 2
Messages
Total messages: 13 (0 generated)
Al, I think this might be worthwhile as an additional simplification?
I think this will be okay but seeing some try runs for all our various linux configurations would be comforting. The browser tests should blow up if there are issues with not initializing GDK. LGTM if that works. https://chromiumcodereview.appspot.com/10078018/diff/1/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): https://chromiumcodereview.appspot.com/10078018/diff/1/content/gpu/gpu_main.c... content/gpu/gpu_main.cc:38: #include "ui/gfx/gtk_util.h" I think you should be able to delete this.
On 2012/04/19 19:54:10, apatrick_chromium wrote: > I think this will be okay but seeing some try runs for all our various linux > configurations would be comforting. The browser tests should blow up if there > are issues with not initializing GDK. LGTM if that works. > > https://chromiumcodereview.appspot.com/10078018/diff/1/content/gpu/gpu_main.cc > File content/gpu/gpu_main.cc (right): > > https://chromiumcodereview.appspot.com/10078018/diff/1/content/gpu/gpu_main.c... > content/gpu/gpu_main.cc:38: #include "ui/gfx/gtk_util.h" > I think you should be able to delete this. Done.
I see GLX failures on linux_chromeos. extension "GLX" missing on display ":9.0". [13284:13284:0419/132813:2265785398:ERROR:gl_surface_glx.cc(60)] glxQueryVersion failed [13284:13284:0419/132813:2265785420:ERROR:gl_surface_linux.cc(55)] GLSurfaceGLX::InitializeOneOff failed. [13284:13284:0419/132813:2265785432:ERROR:compositor.cc(72)] Could not load the GL bindings http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds... However, this seems to always happen on linux_chromeos, so LGTM.
LGTM too.
On 2012/04/20 17:57:41, kbr wrote: > LGTM too. Thx. TBR=darin and landing because none of the three of us are base/ OWNER.
Are we using message_pump_gtk.cc in the GPU process?? https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc File base/message_pump_gtk.cc (right): https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.... base/message_pump_gtk.cc:89: Display* MessagePumpGtk::GetDefaultXDisplay() { why do we need to involve a class named *Gtk in code that should never be using GTK? can you say more about why this change is needed?
On Mon, Apr 23, 2012 at 9:52 AM, <darin@chromium.org> wrote: > Are we using message_pump_gtk.cc in the GPU process?? > We actually were until I landed a separate change last week. I think maybe the GPU process used to be architected differently (defer to Ken, Al) but it certainly doesn't need it any more. > > https://chromiumcodereview.**appspot.com/10078018/diff/1/** > base/message_pump_gtk.cc<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc> > File base/message_pump_gtk.cc (right): > > https://chromiumcodereview.**appspot.com/10078018/diff/1/** > base/message_pump_gtk.cc#**newcode89<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc#newcode89> > base/message_pump_gtk.cc:89: Display* > MessagePumpGtk::**GetDefaultXDisplay() { > why do we need to involve a class named *Gtk in code that should never > be using GTK? can you say more about why this change is needed? > I would prefer to use MessagePumpX::GetDefaultXDisplay() but which one of these things you land at seems to be a compile decision regarding the typedef of MessagePumpForUI. Linux desktop is overall a "gtk" build so I'm not sure it's legal to mix and match the two underlying pumps on a per-source-file basis? My change is heading towards a world where the GPU process doesn't land in GDK or GTK code ever. I think we're there now, with this remaining naming ugly. > https://chromiumcodereview.**appspot.com/1007<https://chromiumcodereview.apps... *Are we using message_pump_gtk.cc in the GPU process??* 8018/ <https://chromiumcodereview.appspot.com/10078018/> >
On Mon, Apr 23, 2012 at 4:36 PM, Chris Evans <cevans@google.com> wrote: > On Mon, Apr 23, 2012 at 9:52 AM, <darin@chromium.org> wrote: > >> Are we using message_pump_gtk.cc in the GPU process?? >> > > We actually were until I landed a separate change last week. I think maybe > the GPU process used to be architected differently (defer to Ken, Al) but > it certainly doesn't need it any more. > There is a type enum that you can use to decide what kind of pump to allocate when you create a MessageLoop. If there is shared code that needs to be a GTK message loop sometimes and other times a default message loop, then we could perhaps parameterize the code accordingly. MessageLoop::InitMessagePumpForUIFactory seems to be another way to alter the constructed MessagePump for a process. > > >> >> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >> base/message_pump_gtk.cc<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc> >> File base/message_pump_gtk.cc (right): >> >> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >> base/message_pump_gtk.cc#**newcode89<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc#newcode89> >> base/message_pump_gtk.cc:89: Display* >> MessagePumpGtk::**GetDefaultXDisplay() { >> why do we need to involve a class named *Gtk in code that should never >> be using GTK? can you say more about why this change is needed? >> > > I would prefer to use MessagePumpX::GetDefaultXDisplay() but which one of > these things you land at seems to be a compile decision regarding the > typedef of MessagePumpForUI. Linux desktop is overall a "gtk" build so I'm > not sure it's legal to mix and match the two underlying pumps on a > per-source-file basis? > We can't just change the calling code to call XOpenDisplay(NULL) directly? > > My change is heading towards a world where the GPU process doesn't land in > GDK or GTK code ever. I think we're there now, with this remaining naming > ugly. > > >> https://chromiumcodereview.**appspot.com/1007<https://chromiumcodereview.apps... > > *Are we using message_pump_gtk.cc in the GPU process??* > > 8018/ <https://chromiumcodereview.appspot.com/10078018/> >> > >
On Mon, Apr 23, 2012 at 8:05 PM, Darin Fisher <darin@chromium.org> wrote: > On Mon, Apr 23, 2012 at 4:36 PM, Chris Evans <cevans@google.com> wrote: > >> On Mon, Apr 23, 2012 at 9:52 AM, <darin@chromium.org> wrote: >> >>> Are we using message_pump_gtk.cc in the GPU process?? >>> >> >> We actually were until I landed a separate change last week. I think >> maybe the GPU process used to be architected differently (defer to Ken, Al) >> but it certainly doesn't need it any more. >> > > There is a type enum that you can use to decide what kind of pump to > allocate when you create a MessageLoop. > Yes, that's what I changed last week. The old type was "UI" -- perhaps required historically but not any more. I backed it down to "DEFAULT" -- bye bye GTK. If there is shared code that needs to be a GTK message loop sometimes and > other times a default message loop, then we could perhaps parameterize the > code accordingly. > > MessageLoop::InitMessagePumpForUIFactory seems to be another way to alter > the constructed MessagePump for a process. > > > >> >> >>> >>> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >>> base/message_pump_gtk.cc<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc> >>> File base/message_pump_gtk.cc (right): >>> >>> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >>> base/message_pump_gtk.cc#**newcode89<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc#newcode89> >>> base/message_pump_gtk.cc:89: Display* >>> MessagePumpGtk::**GetDefaultXDisplay() { >>> why do we need to involve a class named *Gtk in code that should never >>> be using GTK? can you say more about why this change is needed? >>> >> >> I would prefer to use MessagePumpX::GetDefaultXDisplay() but which one of >> these things you land at seems to be a compile decision regarding the >> typedef of MessagePumpForUI. Linux desktop is overall a "gtk" build so I'm >> not sure it's legal to mix and match the two underlying pumps on a >> per-source-file basis? >> > > We can't just change the calling code to call XOpenDisplay(NULL) directly? > It used to be this way but apparently it led to https://code.google.com/p/chromium/issues/detail?id=104248 and the fix was to centralize access to the Display* on the MessagePumpForUI object. > >> >> My change is heading towards a world where the GPU process doesn't land >> in GDK or GTK code ever. I think we're there now, with this remaining >> naming ugly. >> >> >>> https://chromiumcodereview.**appspot.com/1007<https://chromiumcodereview.apps... >> >> *Are we using message_pump_gtk.cc in the GPU process??* >> >> 8018/ <https://chromiumcodereview.appspot.com/10078018/> >>> >> >> >
On Mon, Apr 23, 2012 at 10:08 PM, Chris Evans <cevans@google.com> wrote: > On Mon, Apr 23, 2012 at 8:05 PM, Darin Fisher <darin@chromium.org> wrote: > >> On Mon, Apr 23, 2012 at 4:36 PM, Chris Evans <cevans@google.com> wrote: >> >>> On Mon, Apr 23, 2012 at 9:52 AM, <darin@chromium.org> wrote: >>> >>>> Are we using message_pump_gtk.cc in the GPU process?? >>>> >>> >>> We actually were until I landed a separate change last week. I think >>> maybe the GPU process used to be architected differently (defer to Ken, Al) >>> but it certainly doesn't need it any more. >>> >> >> There is a type enum that you can use to decide what kind of pump to >> allocate when you create a MessageLoop. >> > > Yes, that's what I changed last week. The old type was "UI" -- perhaps > required historically but not any more. I backed it down to "DEFAULT" -- > bye bye GTK. > Good :) > > If there is shared code that needs to be a GTK message loop sometimes and >> other times a default message loop, then we could perhaps parameterize the >> code accordingly. >> >> MessageLoop::InitMessagePumpForUIFactory seems to be another way to alter >> the constructed MessagePump for a process. >> >> >> >>> >>> >>>> >>>> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >>>> base/message_pump_gtk.cc<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc> >>>> File base/message_pump_gtk.cc (right): >>>> >>>> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >>>> base/message_pump_gtk.cc#**newcode89<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc#newcode89> >>>> base/message_pump_gtk.cc:89: Display* >>>> MessagePumpGtk::**GetDefaultXDisplay() { >>>> why do we need to involve a class named *Gtk in code that should never >>>> be using GTK? can you say more about why this change is needed? >>>> >>> >>> I would prefer to use MessagePumpX::GetDefaultXDisplay() but which one >>> of these things you land at seems to be a compile decision regarding the >>> typedef of MessagePumpForUI. Linux desktop is overall a "gtk" build so I'm >>> not sure it's legal to mix and match the two underlying pumps on a >>> per-source-file basis? >>> >> >> We can't just change the calling code to call XOpenDisplay(NULL) directly? >> > > It used to be this way but apparently it led to > https://code.google.com/p/chromium/issues/detail?id=104248 and the fix > was to centralize access to the Display* on the MessagePumpForUI object. > > Doesn't it seem strange to ask the MessagePumpGtk for the X Display handle? It seems like if MessagePumpGtk needs to know the X Display handle and other code unrelated to GTK needs to the X Display handle that there should be some lower level common way to get the X Display handle that both can use. Maybe this is worth mentioning to piman since he authored http://codereview.chromium.org/8890042/. -Darin > >> >>> >>> My change is heading towards a world where the GPU process doesn't land >>> in GDK or GTK code ever. I think we're there now, with this remaining >>> naming ugly. >>> >>> >>>> https://chromiumcodereview.**appspot.com/1007<https://chromiumcodereview.apps... >>> >>> *Are we using message_pump_gtk.cc in the GPU process??* >>> >>> 8018/ <https://chromiumcodereview.appspot.com/10078018/> >>>> >>> >>> >> >
On Tue, Apr 24, 2012 at 11:39 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Mon, Apr 23, 2012 at 10:08 PM, Chris Evans <cevans@google.com> wrote: > >> On Mon, Apr 23, 2012 at 8:05 PM, Darin Fisher <darin@chromium.org> wrote: >> >>> On Mon, Apr 23, 2012 at 4:36 PM, Chris Evans <cevans@google.com> wrote: >>> >>>> On Mon, Apr 23, 2012 at 9:52 AM, <darin@chromium.org> wrote: >>>> >>>>> Are we using message_pump_gtk.cc in the GPU process?? >>>>> >>>> >>>> We actually were until I landed a separate change last week. I think >>>> maybe the GPU process used to be architected differently (defer to Ken, Al) >>>> but it certainly doesn't need it any more. >>>> >>> >>> There is a type enum that you can use to decide what kind of pump to >>> allocate when you create a MessageLoop. >>> >> >> Yes, that's what I changed last week. The old type was "UI" -- perhaps >> required historically but not any more. I backed it down to "DEFAULT" -- >> bye bye GTK. >> > > Good :) > > >> >> If there is shared code that needs to be a GTK message loop sometimes and >>> other times a default message loop, then we could perhaps parameterize the >>> code accordingly. >>> >>> MessageLoop::InitMessagePumpForUIFactory seems to be another way to >>> alter the constructed MessagePump for a process. >>> >>> >>> >>>> >>>> >>>>> >>>>> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >>>>> base/message_pump_gtk.cc<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc> >>>>> File base/message_pump_gtk.cc (right): >>>>> >>>>> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >>>>> base/message_pump_gtk.cc#**newcode89<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc#newcode89> >>>>> base/message_pump_gtk.cc:89: Display* >>>>> MessagePumpGtk::**GetDefaultXDisplay() { >>>>> why do we need to involve a class named *Gtk in code that should never >>>>> be using GTK? can you say more about why this change is needed? >>>>> >>>> >>>> I would prefer to use MessagePumpX::GetDefaultXDisplay() but which one >>>> of these things you land at seems to be a compile decision regarding the >>>> typedef of MessagePumpForUI. Linux desktop is overall a "gtk" build so I'm >>>> not sure it's legal to mix and match the two underlying pumps on a >>>> per-source-file basis? >>>> >>> >>> We can't just change the calling code to call XOpenDisplay(NULL) >>> directly? >>> >> >> It used to be this way but apparently it led to >> https://code.google.com/p/chromium/issues/detail?id=104248 and the fix >> was to centralize access to the Display* on the MessagePumpForUI object. >> >> > Doesn't it seem strange to ask the MessagePumpGtk for the X Display > handle? It seems like if MessagePumpGtk needs to know the X Display handle > and other code unrelated to GTK needs to the X Display handle that there > should be some lower level common way to get the X Display handle that both > can use. > I agree. > > Maybe this is worth mentioning to piman since he authored > http://codereview.chromium.org/8890042/. > I don't mind taking care of it. I filed https://code.google.com/p/chromium/issues/detail?id=125201 and will have a look in a few weeks when I'm back from vacation. Cheers Chris > -Darin > > > >> >>> >>>> >>>> My change is heading towards a world where the GPU process doesn't land >>>> in GDK or GTK code ever. I think we're there now, with this remaining >>>> naming ugly. >>>> >>>> >>>>> https://chromiumcodereview.**appspot.com/1007<https://chromiumcodereview.apps... >>>> >>>> *Are we using message_pump_gtk.cc in the GPU process??* >>>> >>>> 8018/ <https://chromiumcodereview.appspot.com/10078018/> >>>>> >>>> >>>> >>> >> >
On Thu, Apr 26, 2012 at 10:22 AM, Chris Evans <cevans@google.com> wrote: > On Tue, Apr 24, 2012 at 11:39 PM, Darin Fisher <darin@chromium.org> wrote: > >> >> >> On Mon, Apr 23, 2012 at 10:08 PM, Chris Evans <cevans@google.com> wrote: >> >>> On Mon, Apr 23, 2012 at 8:05 PM, Darin Fisher <darin@chromium.org>wrote: >>> >>>> On Mon, Apr 23, 2012 at 4:36 PM, Chris Evans <cevans@google.com> wrote: >>>> >>>>> On Mon, Apr 23, 2012 at 9:52 AM, <darin@chromium.org> wrote: >>>>> >>>>>> Are we using message_pump_gtk.cc in the GPU process?? >>>>>> >>>>> >>>>> We actually were until I landed a separate change last week. I think >>>>> maybe the GPU process used to be architected differently (defer to Ken, Al) >>>>> but it certainly doesn't need it any more. >>>>> >>>> >>>> There is a type enum that you can use to decide what kind of pump to >>>> allocate when you create a MessageLoop. >>>> >>> >>> Yes, that's what I changed last week. The old type was "UI" -- perhaps >>> required historically but not any more. I backed it down to "DEFAULT" -- >>> bye bye GTK. >>> >> >> Good :) >> >> >>> >>> If there is shared code that needs to be a GTK message loop sometimes >>>> and other times a default message loop, then we could perhaps parameterize >>>> the code accordingly. >>>> >>>> MessageLoop::InitMessagePumpForUIFactory seems to be another way to >>>> alter the constructed MessagePump for a process. >>>> >>>> >>>> >>>>> >>>>> >>>>>> >>>>>> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >>>>>> base/message_pump_gtk.cc<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc> >>>>>> File base/message_pump_gtk.cc (right): >>>>>> >>>>>> https://chromiumcodereview.**appspot.com/10078018/diff/1/** >>>>>> base/message_pump_gtk.cc#**newcode89<https://chromiumcodereview.appspot.com/10078018/diff/1/base/message_pump_gtk.cc#newcode89> >>>>>> base/message_pump_gtk.cc:89: Display* >>>>>> MessagePumpGtk::**GetDefaultXDisplay() { >>>>>> why do we need to involve a class named *Gtk in code that should never >>>>>> be using GTK? can you say more about why this change is needed? >>>>>> >>>>> >>>>> I would prefer to use MessagePumpX::GetDefaultXDisplay() but which one >>>>> of these things you land at seems to be a compile decision regarding the >>>>> typedef of MessagePumpForUI. Linux desktop is overall a "gtk" build so I'm >>>>> not sure it's legal to mix and match the two underlying pumps on a >>>>> per-source-file basis? >>>>> >>>> >>>> We can't just change the calling code to call XOpenDisplay(NULL) >>>> directly? >>>> >>> >>> It used to be this way but apparently it led to >>> https://code.google.com/p/chromium/issues/detail?id=104248 and the fix >>> was to centralize access to the Display* on the MessagePumpForUI object. >>> >>> >> Doesn't it seem strange to ask the MessagePumpGtk for the X Display >> handle? It seems like if MessagePumpGtk needs to know the X Display handle >> and other code unrelated to GTK needs to the X Display handle that there >> should be some lower level common way to get the X Display handle that both >> can use. >> > > I agree. > > >> >> Maybe this is worth mentioning to piman since he authored >> http://codereview.chromium.org/8890042/. >> > > I don't mind taking care of it. I filed > https://code.google.com/p/chromium/issues/detail?id=125201 and will have > a look in a few weeks when I'm back from vacation. > > OK, thanks! > > Cheers > Chris > > >> -Darin >> >> >> >>> >>>> >>>>> >>>>> My change is heading towards a world where the GPU process doesn't >>>>> land in GDK or GTK code ever. I think we're there now, with this remaining >>>>> naming ugly. >>>>> >>>>> >>>>>> https://chromiumcodereview.**appspot.com/1007<https://chromiumcodereview.apps... >>>>> >>>>> *Are we using message_pump_gtk.cc in the GPU process??* >>>>> >>>>> 8018/ <https://chromiumcodereview.appspot.com/10078018/> >>>>>> >>>>> >>>>> >>>> >>> >> > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
