Bug 173153 – [CoordinatedGraphics] Behavior change of DrawingAreaImpl after r217779

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Daewoong Jang]

By changeset 217779(https://bugs.webkit.org/show_bug.cgi?id=172496), DrawingAreaImpl::mainFrameContentSizeChanged() was moved to AcceleratedDrawingArea::mainFrameContentSizeChanged(). However the move has caused behavior change of DrawingAreaImpl, as it revives the existing implementation of AcceleratedDrawingArea which was shadowed by both DrawingAreaImpl and DrawingAreaWPE before the change.

It seems that the behavior change is unintended.

[Daewoong Jang]

Created attachment 312427 [details]
patch

[Carlos Garcia Campos]

Has this broken something? I think it was the previous code what was wrong, we probably didn't notice it because we don't expose fixed layout.

[Daewoong Jang]

(In reply to Carlos Garcia Campos from comment #2)
> Has this broken something? I think it was the previous code what was wrong,
> we probably didn't notice it because we don't expose fixed layout.

I'm using fixed layout and CoordinatedGraphics stopped displaying pages since the patch. I'd appreciate if you could investigate further. Thanks for reply!

[Carlos Garcia Campos]

Comment on attachment 312427 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312427&action=review

> Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:170
>      if (m_webPage.useFixedLayout()) {
>          if (m_layerTreeHost)
>              m_layerTreeHost->sizeDidChange(size);
>          else if (m_previousLayerTreeHost)
>              m_previousLayerTreeHost->sizeDidChange(size);

Ok, this comes from CoordinatedDrawingArea, it was merged into AcceleratedDrawingArea, but never used by us because the DrawingAreaImpl never chained up. I'm not sure if that was on purpose because this was only used by the multiprocess coordinated graphics implementation or if it was a bug, but since we only have threaded coordinated graphics now, I think we can simply remove this.

> Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:172
>      m_webPage.mainFrame()->pageOverlayController().didChangeDocumentSize();

And this can be removed too, since FrameView already takes care of this.

[Daewoong Jang]

(In reply to Carlos Garcia Campos from comment #4)
> Comment on attachment 312427 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312427&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:170
> >      if (m_webPage.useFixedLayout()) {
> >          if (m_layerTreeHost)
> >              m_layerTreeHost->sizeDidChange(size);
> >          else if (m_previousLayerTreeHost)
> >              m_previousLayerTreeHost->sizeDidChange(size);
> 
> Ok, this comes from CoordinatedDrawingArea, it was merged into
> AcceleratedDrawingArea, but never used by us because the DrawingAreaImpl
> never chained up. I'm not sure if that was on purpose because this was only
> used by the multiprocess coordinated graphics implementation or if it was a
> bug, but since we only have threaded coordinated graphics now, I think we
> can simply remove this.
> 
> > Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:172
> >      m_webPage.mainFrame()->pageOverlayController().didChangeDocumentSize();
> 
> And this can be removed too, since FrameView already takes care of this.

Alright. I will update the patch as you suggested. Thank you for review.

[Daewoong Jang]

Created attachment 312656 [details]
updated patch

[Carlos Garcia Campos]

Comment on attachment 312656 [details]
updated patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 312656 [details]
updated patch

Clearing flags on attachment: 312656

Committed r218102: <http://trac.webkit.org/changeset/218102>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.