Bug 77155 – [chromium] GL_CHROMIUM_gpu_memory_manager extension

• Component: New Bugs
• Status: RESOLVED FIXED

[Michal Mocny]

[chromium] Adding gl extensions for addAffectedRenderWidgetById and setMemoryAllocationChangedCallback

[Michal Mocny]

Created attachment 124191 [details]
Patch

[Michal Mocny]

Will need to wait for http://codereview.chromium.org/9289052/ work to land before this can go through.

[Nat Duca]

Comment on attachment 124191 [details]
Patch

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

You didn't do the Extensions3DChromium plumbing. Please do that as well in this patch.

> Source/WebKit/chromium/ChangeLog:2
> +

Your commit message should just be

Add support for GL_xxx_CHROMIUM

What is our extension? I think its one extension, GL_gpu_memory_manager_CHROMIUM

> Source/WebKit/chromium/ChangeLog:21
> +        (WebKit::WebGraphicsContext3D::addAffectedRenderWidgetByIdCHROMIUM):

See my comments on the chrome side bug about:
1) clarifying the behavior of WGC3D::Initialize() and whether that always implies the passed-in WebView is affected if its non-null?
2) using WebViews instead of renderwidget (both in naming and argument)
3) I feel like I had a comment #3 but I forgot :/

[Michal Mocny]

Created attachment 125023 [details]
Patch

[Michal Mocny]

This is not finished (I am going to change the raw pointer usage to Owning/Passing pointers, etc).

However, I wanted feedback specifically if it was okay to predeclare the two WebKit classes in Extensions3DChromium.h which is in WebCore.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 125023 [details]
Patch

Attachment 125023 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11396607

[James Robinson]

Comment on attachment 125023 [details]
Patch

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

Can you put a description somewhere of what this does?

> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:97
> +    virtual void addAffectedWebViewCHROMIUM(WebKit::WebView* webview);
> +    virtual void removeAffectedWebViewCHROMIUM(WebKit::WebView* webview);

you can't talk about namespace WebKit concepts in WebCore, it's a layering violation.

> Source/WebKit/chromium/public/platform/WebGpuMemoryAllocation.h:2
> + * Copyright (C) 2009 Google Inc. All rights reserved.

2012

> Source/WebKit/chromium/public/platform/WebGpuMemoryAllocation.h:8
> + *     * Redistributions of source code must retain the above copyright

we use 2-clause license header for new files

> Source/WebKit/chromium/public/platform/WebGpuMemoryAllocation.h:37
> +#include <content/common/gpu/gpu_memory_allocation.h>

this is also a layering violation. WebKit should not depend on content

[Michal Mocny]

Created attachment 125184 [details]
Patch

[Nat Duca]

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

Looks close!

>> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:97
>> +    virtual void removeAffectedWebViewCHROMIUM(WebKit::WebView* webview);
> 
> you can't talk about namespace WebKit concepts in WebCore, it's a layering violation.

I dont think we need these two methods exposed. The WebViewImpl is the only one that needs that, and it can do it by reaching inside the SharedGraphicsContext and calling directly into the underlyign WGC3D.

> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:100
> +        virtual void onMemoryAllocationChanged(const WebKit::WebGpuMemoryAllocation&) = 0;

The extensions3dchromium should define an inner struct that is the allocation.

GraphicsContext3DCHromium should convert from the Web allocation to the extensions versoin.

> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:103
> +    virtual void setMemoryAllocationChangedCallbackCHROMIUM(GraphicsMemoryAllocationChangedCallbackCHROMIUM*);

copy the style of swapcomplete for pointer ownership

>> Source/WebKit/chromium/public/platform/WebGpuMemoryAllocation.h:37
>> +#include <content/common/gpu/gpu_memory_allocation.h>
> 
> this is also a layering violation. WebKit should not depend on content

This is copying the style of WebRect.h, which does this. Thoughts, james?

Barring this, or if you simply desire to make progress fast (which I think makes sense) make this structure oblivious of GpuMemoryAllocation and then in WebGraphicsContext3DCommandBufferImpl convert to its version of the struct by hand.

> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:162
> +    virtual void addAffectedWebViewCHROMIUM(WebView* webview) { }

style bot is going to complain at giving this a name, just say (WebView* )

[Dana Jansens]

Comment on attachment 125184 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebGpuMemoryAllocation.h:2
> + * Copyright (C) 2009 Google Inc. All rights reserved.

2012

[James Robinson]

Comment on attachment 125184 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:163
> +    virtual void addAffectedWebViewCHROMIUM(WebView* webview) { }
> +    virtual void removeAffectedWebViewCHROMIUM(WebView* webview) { }

still NACK on layering - stuff in the platform API (WK/chromium/public/platform) is not allowed to be aware of types in the client API (WK/chromium/public). This is so the implementation of the platform API, which the rest of WebKit depends on, can be built without having knowledge of the higher-level client API. Otherwise we'd have a cycle.

[James Robinson]

(In reply to comment #10)
> >> Source/WebKit/chromium/public/platform/WebGpuMemoryAllocation.h:37
> >> +#include <content/common/gpu/gpu_memory_allocation.h>
> > 
> > this is also a layering violation. WebKit should not depend on content
> 
> This is copying the style of WebRect.h, which does this. Thoughts, james?
> 

WebRect is different, it's depending on a ui/gfx type which is very near to the lowest level of our dependency tree.  That's OK, but content is right out.

30 second guide to layers we try to maintain in chromium / WebKit from top to bottom:

"chrome" namespace chrome: src/chrome/** in chromium checkout, implementation of chromium browser features (like new tab page, extensions, sync)

"content" namespace content: src/content/** in chromium checkout, implementation of multi-process view handling. aware of multiple processes, IPC, and how to embed WebKit but no knowledge of chromium browser features

"WebKit client API" namespace WebKit: Source/WebKit/chromium/public/* in WebKit checkout, WebKit client embedding API. single-process embedding API for WebKit. isolates all chromium code from WebKit internal types. not aware of IPC

"WebCore" namespace WebCore: Source/WebCore/** in WebKit checkout. implementation of all sorts of web stuff

"WebKit platform API" namespace WebKit (confusing i know): Source/WebKit/chromium/public/platform/* in WebKit checkout, WebKit platform API. This is the API for all platform constructs that WebKit depends on - things like network stack, graphics stack, etc.  implementations of this platform API should not depend on WebKit (and thus not depend on anything in the WebKit client API)




From any layer you can depend on nearly anything below you - for example code in chrome/ or content/ can use the WebKit client API or the WebKit platform APIs directly - but you can never depend or use types from layers above you.

[Michal Mocny]

Thanks James.  I was aware of the layers down to the WebKit platform but did not understand that distinction (I thought WebCore was the lowest..).

(In reply to comment #13)
> (In reply to comment #10)
> > >> Source/WebKit/chromium/public/platform/WebGpuMemoryAllocation.h:37
> > >> +#include <content/common/gpu/gpu_memory_allocation.h>
> > > 
> > > this is also a layering violation. WebKit should not depend on content
> > 
> > This is copying the style of WebRect.h, which does this. Thoughts, james?
> > 
> 
> WebRect is different, it's depending on a ui/gfx type which is very near to the lowest level of our dependency tree.  That's OK, but content is right out.
> 
> 30 second guide to layers we try to maintain in chromium / WebKit from top to bottom:
> 
> "chrome" namespace chrome: src/chrome/** in chromium checkout, implementation of chromium browser features (like new tab page, extensions, sync)
> 
> "content" namespace content: src/content/** in chromium checkout, implementation of multi-process view handling. aware of multiple processes, IPC, and how to embed WebKit but no knowledge of chromium browser features
> 
> "WebKit client API" namespace WebKit: Source/WebKit/chromium/public/* in WebKit checkout, WebKit client embedding API. single-process embedding API for WebKit. isolates all chromium code from WebKit internal types. not aware of IPC
> 
> "WebCore" namespace WebCore: Source/WebCore/** in WebKit checkout. implementation of all sorts of web stuff
> 
> "WebKit platform API" namespace WebKit (confusing i know): Source/WebKit/chromium/public/platform/* in WebKit checkout, WebKit platform API. This is the API for all platform constructs that WebKit depends on - things like network stack, graphics stack, etc.  implementations of this platform API should not depend on WebKit (and thus not depend on anything in the WebKit client API)
> 
> 
> 
> 
> From any layer you can depend on nearly anything below you - for example code in chrome/ or content/ can use the WebKit client API or the WebKit platform APIs directly - but you can never depend or use types from layers above you.

[Michal Mocny]

Created attachment 125336 [details]
Patch

[Nat Duca]

James, your layering argument above neglects to acknowledge that the platform API is WiP. Right now, WebGraphicsContext3D still uses the WebView* as a method to create view contexts. It is the only way to identify views back to the embedder, at this point.

We need to eventually evolve that to use some other ID, perhaps surface ids, but that plumbing isn't ready in Chrome land either. When I finish my inversion, it should be, but its not there yet.

In the interm, I think that use of WebView* on WebGraphicsContext3D is an acceptable near-term violation.

[Nat Duca]

Specifically, the way to fix this probably is to use int32 surface_id everywhere

The new WebWidgetSurface class would have a surface_id accessor.

WebGraphcisContext3D would be constructable with surface ids.

The methods Michal is adding here would use surface ids.

Doing this right now, just strikes me as a little painful. We're trying to keep this stuff cherry-pickable.

[James Robinson]

I'd like to understand a bit better what this patch is trying to accomplish in order to figure out the best way to do it.  We already have an association between WebGraphicsContext3D instances and WebViews for compositor contexts, via the initialization path.

Is this API supposed to be used for non-compositor contexts?  If so, what is the meaning of this association?  I think such an association will be difficult to make in general since a single canvas 2d or webgl context can contribute to many different views, possibly at the same time. It can also contribute to views that aren't composited (consider an offscreen webgl context used for some computation).

[Michal Mocny]

Created attachment 126345 [details]
Patch

[James Robinson]

I'd like to see more motivation behind the need to associated contexts with surfaces and how different sorts of contexts are intended to use this, either here or in a design doc.  I don't feel as if comment #18 has been addressed sufficiently to move forward this this patch as is.

[Nat Duca]

(In reply to comment #20)
> I'd like to see more motivation behind the need to associated contexts with surfaces and how different sorts of contexts are intended to use this, either here or in a design doc.  I don't feel as if comment #18 has been addressed sufficiently to move forward this this patch as is.

I think your confusion is about the affected context being the same as the webview. That is definitely not the case.

The issue here is the shared graphics context. That context contains resources that affect some webviews, but not others. Our goal is to free up Ganesh resources when all of the WebView's that use Ganesh are invisible [in the near term] and in the medium term, to dynamically adjust the allocation given to ganesh based on whether there is a canvas in the most immediate foreground tab versus in a background tab.

To do this, we need to have a list for the shared graphics context what WebView's it affects. @twiz has a patch to do this by scanning during commit for a canvas2d layer backed by Ganesh. When such a layer exists, a post-commit hook uses this information to tell the *shared graphics context* that it affects the *compositor's context*.

Hopefully this clears up why the WebViewImpl<->GraphicsContext3D association isn't valid here.

Now, the reason that this patch uses surface IDs instead of pointers is about thread safety. The place where we will add/remove affected surfaces is on the main thread. Right now, it is being done in a post-commit step (see @twiz's patch). While in the very very near term, we migth make an assumpiton that the pointers are thread-safe, in the not-very-long term, I think that's an unsafe assumption.

Thus, we need a way to refer from the sharedgraphicscontext to the webview's onscreen context. One layerdown the chrome-side of the graphics stack, all this webviewimpl<->renderwidgetost mappings turn into surface ids, which are plain old int32s. This thus seems like a natural way to identify things --- the WebViewImpls stores their surface_id's and then twiz's patch can hand that surface id to the shared graphics contexts whenever a webviewimpl starts having an accelerated canvas.

I hope this helps. I really do think that this is the right way forward.

[Nat Duca]

Comment on attachment 126345 [details]
Patch

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

Unofficially this LGTM. Though, I admit that this is because I've been working on this with Michael, Twiz and so on for a while now. :)

James, I hope my previous notes address your concerns sufficiently that we can get this landed. If not, I'm happy to work with you more to see what the disconnect is.

piman, I hope that it is clear from my previous comments why using surface_id instead of a WebGraphicsConext3D* on the add/remove functions was done. To reiterate, my concern here is that pointer access makes the assumption that the impl-thread-side context is safely accessible. Personally, I'd prefer to avoid that assumption --- the weak-nature of the surface_id's allows that to happen. Does this make sense to you?

> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:100
> +    void addAffectedSurfaceCHROMIUM(int);

Surface->SurfaceID

> Source/WebKit/chromium/public/WebViewClient.h:347
> +    // Memory Management ---------------------------------------------------

How about making an accessor on WebGraphicsContext3D for surfaceId()?

> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:163
> +    virtual void removeAffectedSurfaceCHROMIUM(int) { }

Any reason for making these {} instead of pure =0? Is the intent to land this before the other side? I was leaning toward landing the chrome patches then these...

[James Robinson]

(In reply to comment #21)
> (In reply to comment #20)
> > I'd like to see more motivation behind the need to associated contexts with surfaces and how different sorts of contexts are intended to use this, either here or in a design doc.  I don't feel as if comment #18 has been addressed sufficiently to move forward this this patch as is.
> 
> I think your confusion is about the affected context being the same as the webview. That is definitely not the case.
> 
> The issue here is the shared graphics context. That context contains resources that affect some webviews, but not others. Our goal is to free up Ganesh resources when all of the WebView's that use Ganesh are invisible [in the near term] and in the medium term, to dynamically adjust the allocation given to ganesh based on whether there is a canvas in the most immediate foreground tab versus in a background tab.

I don't think that the issue is the shared graphics context - the issue is that *every* graphics context is shared in the sense that it might contribute to many views.  A single WebGL context might drive 5 tabs, for example, via drawImage().  How is that relationship represented here?  What if some of the tabs are driven by a context even if that context doesn't directly contribute to the compositor in that tab (see more below)?
> 
> To do this, we need to have a list for the shared graphics context what WebView's it affects. @twiz has a patch to do this by scanning during commit for a canvas2d layer backed by Ganesh. When such a layer exists, a post-commit hook uses this information to tell the *shared graphics context* that it affects the *compositor's context*.

I don't think that is sufficient - canvas might be used in a certain web view even if it doesn't generate a layer (and in fact I think this is pretty common).  For example, a WebGL game may generate some set of resources using offscreen canvases and then upload them to textures.  A page might do significant work in a WebGL or canvas context that's completely offscreen, reading data back through readback mechanisms.  We won't be able to detect either of these cases by examining only the compositing tree, but I think they're common enough to worry about (maps definitely does the first).

To look at this from another angle, today we strongly associated compositor contexts with views (via creation).  For all other contexts we express the relationship between them via share groups (ignoring pepper for the moment where we have a different sharing mechanism).  Looked at from that angle, we can define a set of WebViews a context may contribute to by looking at all contexts in its share group and identifying the compositor / onscreen contexts.  Then a context is fully "backgrounded" when all of the WebViews it is associated with are hidden.

Today, that would have the practical effect that we treat everything in the same process as linked with everything else (because they are in the share group).  I think that's a realistic view of what knowledge we have about how the contexts may be interacting with each other - if they are in the same render process, their might be JS links between them that allow for direct manipulation of each other's resources.  If we can get a tighter view of exactly which WebGL contexts may have JS links with others then we can use that information to make smaller GL share groups which would also tighten up the set of contexts that are related to each view.

This also has the benefit of not needing any new API since we already have API for share groups.

Hope this makes sense.

[Nat Duca]

I disagree with your point of view.

Using sharegroups, a foreground plain-old-webpage in one tab would have reduced memory allocation simply because a page in the background happened to have briefly created a canvas context. That seems very wrong.

I understand that you are taking a conservative route ("all contexts affect all share groups"). But, I don't understand your resistance to another API --- APIs aren't themselves costly. In this case, they give us a future avenue to be more precise in our allocations.

We can in some cases attribute a canvas or webgl instance to specific context. We can't always be 100% right, but the entire memory managment scheme is suggestive, not mandatory. Mistakes are fine because the entire allocation system is based on "suggestion" not enforcement -- you can go over your allocation without penalty.

I'll say it one more time: we want to give truly visible tabs the most memory they can get, *and* we want to provide an *avenue* to minimize the memory allocations given to invisible tabs.


I would last-ditch propose that we go ahead with the surfacE_id approach. If in the future we decide that share groups are sufficient, we can *simplify*. Right now, I think a reductionist approach seems like the wrong way to go.

[James Robinson]

Comment on attachment 126345 [details]
Patch

I think we'll just have to disagree for now.  I appreciate the energy and passion with which you've been tackling this problem and do value your input, but in this instance I strongly feel that attempting to derive context relationships from the compositing tree is not the way to go.

I suggest that we move forward with the rest of the API and see where that gets us.  I think there's an orchard of low-hanging fruit to keep us busy for some time.  If in the course of that we uncover new data or gain new insights we should use that to inform our direction.

[Michal Mocny]

Comment on attachment 126345 [details]
Patch

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

I've made changes to remove the add/removeAffectedX extension.

>> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:100
>> +    void addAffectedSurfaceCHROMIUM(int);
> 
> Surface->SurfaceID

removed.

>> Source/WebKit/chromium/public/WebViewClient.h:347
>> +    // Memory Management ---------------------------------------------------
> 
> How about making an accessor on WebGraphicsContext3D for surfaceId()?

removed.

>> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:163
>> +    virtual void removeAffectedSurfaceCHROMIUM(int) { }
> 
> Any reason for making these {} instead of pure =0? Is the intent to land this before the other side? I was leaning toward landing the chrome patches then these...

These have been removed, however, the below set...Callback is { } and not pure because derived classes cannot predeclare WebGraphicsMemoryAllocationChangedCallbackCHROMIUM and so cannot provide an implementation without at least one WebKit patch.

[Michal Mocny]

Created attachment 126807 [details]
Patch

[James Robinson]

Comment on attachment 126807 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:137
> +        virtual ~WebGraphicsMemoryAllocationChangedCallbackCHROMIUM() { }

do we expect callers to need to destroy callbacks via a WebGraphicsMemoryAllocationChangedCallbackCHROMIUM* ? for client type interfaces we often make the d'tor protected if ownership is supposed to be maintained by something with knowledge of the concrete implementation. (i see the other callbacks here follow this pattern, but i'm curious as to why)

> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:161
> +    // GL_CHROMIUM_gpu_memory_manager - sets callback for memory allocation changes.

can we document what the semantics of this are? i'm not sure from this description if this is called when the amount of memory we are using changes or when the amount of memory we are supposed to use changes.

> Source/WebKit/chromium/public/platform/WebGraphicsMemoryAllocation.h:34
> +    unsigned int gpuResourceSizeInBytes;

do we really need a struct here if it's just a single number? can we start with a number and expand as we need to? i'm guessing that we expect the set of things passed to change, but any change to this struct will require changes to both webkit and chromium since this is exposed to both so we aren't hiding too much.

if this is representing a number of bytes, i think it should be a size_t. it's not hard to imagine a WebGL game using more than 4gb of gpu resources on a 64 bit machine

[James Robinson]

Overall looks pretty good.  I left some comments about API aesthetics.

[Michal Mocny]

Comment on attachment 126807 [details]
Patch

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

>> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:137
>> +        virtual ~WebGraphicsMemoryAllocationChangedCallbackCHROMIUM() { }
> 
> do we expect callers to need to destroy callbacks via a WebGraphicsMemoryAllocationChangedCallbackCHROMIUM* ? for client type interfaces we often make the d'tor protected if ownership is supposed to be maintained by something with knowledge of the concrete implementation. (i see the other callbacks here follow this pattern, but i'm curious as to why)

We do expect clients with concrete knowledge of the implementation to maintain the ownership, so I've changed to protected, thanks for the pointer.

>> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:161
>> +    // GL_CHROMIUM_gpu_memory_manager - sets callback for memory allocation changes.
> 
> can we document what the semantics of this are? i'm not sure from this description if this is called when the amount of memory we are using changes or when the amount of memory we are supposed to use changes.

done.

>> Source/WebKit/chromium/public/platform/WebGraphicsMemoryAllocation.h:34
>> +    unsigned int gpuResourceSizeInBytes;
> 
> do we really need a struct here if it's just a single number? can we start with a number and expand as we need to? i'm guessing that we expect the set of things passed to change, but any change to this struct will require changes to both webkit and chromium since this is exposed to both so we aren't hiding too much.
> 
> if this is representing a number of bytes, i think it should be a size_t. it's not hard to imagine a WebGL game using more than 4gb of gpu resources on a 64 bit machine

done.  There was actually a leak here, where client was not managing ownership as it should have, which I have fixed.

[Michal Mocny]

Created attachment 127021 [details]
Patch

[James Robinson]

Comment on attachment 127021 [details]
Patch

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

R=me

> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:135
> +        virtual void onMemoryAllocationChanged(size_t gpuResourceSizeInBytes) = 0;

FYI in other places in the WebKit API (but not in this file) we prefer using either willXXX() or doXXX() to make it clearer whether the callback is invoked before or after the event in question.  In this case I think this should be "didMemory..." since it's an async notification.

i think it's probably better to be locally consistent here and leave the name you have, but could you file a bug on these callbacks collectively not following the normal naming conventions?

> Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:136
> +    protected:

nit: newline before 'protected', please

[Michal Mocny]

Created attachment 127028 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127028 [details]
Patch

Rejecting attachment 127028 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11521396

[WebKit Review Bot]

Comment on attachment 127028 [details]
Patch

Rejecting attachment 127028 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 IndexedDB: Invalid dates should not be valid keys

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/11522410

[Nat Duca]

@michal, are we landing this? Looks like its ready to go.

[Michal Mocny]

Created attachment 127580 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127580 [details]
Patch

Clearing flags on attachment: 127580

Committed r108071: <http://trac.webkit.org/changeset/108071>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ilya Tikhonovsky]

Reverted r108071 for reason:

chromium-mac compilation failed

Committed r108074: <http://trac.webkit.org/changeset/108074>

[Michal Mocny]

Fixed the compile issue (virtual keyword used where a non-virtual function was meant).  Chromium-mac compiler (clang, I take it) caught the warning-as-error.

[Michal Mocny]

Created attachment 127599 [details]
Patch

[Michal Mocny]

Created attachment 127601 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127601 [details]
Patch

Clearing flags on attachment: 127601

Committed r108116: <http://trac.webkit.org/changeset/108116>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.